[Serve] Skip routing stats collection for deployments without record_routing_stats#60810
[Serve] Skip routing stats collection for deployments without record_routing_stats#60810abrarsheikh wants to merge 3 commits intomasterfrom
record_routing_stats#60810Conversation
Signed-off-by: abrar <abrar@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable optimization by skipping routing stats collection for deployments that don't define the record_routing_stats method. The implementation is clear and the test updates are appropriate. I've included a few suggestions to enhance maintainability, primarily by refactoring the ReplicaMetadata tuple into a dataclass to make the code more robust against future modifications. I also noted a minor redundancy that could be simplified. Overall, this is a solid improvement.
| has_user_routing_stats_method = ( | ||
| self._user_callable_wrapper is not None | ||
| and self._user_callable_wrapper.has_user_routing_stats_method | ||
| ) |
There was a problem hiding this comment.
self._user_callable_wrapper is initialized in ReplicaBase.__init__ and is not expected to be None at this point. The is not None check is redundant and can be removed to simplify the code.
has_user_routing_stats_method = (
self._user_callable_wrapper.has_user_routing_stats_method
)| for replica in deployment_dict[id]: | ||
| ref = replica.get_actor_handle().initialize_and_get_metadata.remote() | ||
| _, version, _, _, _, _, _, _, _, _ = ray.get(ref) | ||
| _, version, _, _, _, _, _, _, _, _, _ = ray.get(ref) |
There was a problem hiding this comment.
This large tuple unpacking is fragile. If ReplicaMetadata is refactored to a dataclass as suggested in python/ray/serve/_private/replica.py, this can be simplified to be more readable and robust.
metadata = ray.get(ref)
version = metadata.version| actor_handle = ray.get_actor(replica_name, namespace=SERVE_NAMESPACE) | ||
| ref = actor_handle.initialize_and_get_metadata.remote() | ||
| _, version, _, _, _, _, _, _, _, _ = ray.get(ref) | ||
| _, version, _, _, _, _, _, _, _, _, _ = ray.get(ref) |
There was a problem hiding this comment.
This large tuple unpacking is fragile. If ReplicaMetadata is refactored to a dataclass as suggested in python/ray/serve/_private/replica.py, this can be simplified to be more readable and robust.
metadata = ray.get(ref)
version = metadata.version
eicherseiji
left a comment
eicherseiji
left a comment
There was a problem hiding this comment.
Propagates _user_record_routing_stats presence info to ActorReplicaWrapper so _should_record_routing_stats can return early
2 participants
Summary
record_routing_stats.remote()calls to every replica, even when the user hadn't defined arecord_routing_statsmethod on their deployment class. In that case, the remote call simply returns{}, making it pure overhead.has_user_routing_stats_methodboolean toReplicaMetadata, reported during replica initialization. The controller uses this to skip the remote call entirely when the method is absent.Test plan
test_deployment_state.py-- 102 unit tests passtest_controller_recovery.py::test_recover_start_from_replica_actor_names-- 2 tests passtest_metrics.py::test_routing_stats_delay_metric-- updated to definerecord_routing_statson test deploymenttest_metrics.py::test_routing_stats_error_metric-- no changes needed (already defines the method)related to #60680